Add stdlib by katcipis · Pull Request #228 · madlambda/nash

katcipis · 2017-06-30T00:41:41Z

Work in progress

codecov-io · 2017-06-30T01:10:15Z

Codecov Report

Merging #228 into master will increase coverage by 0.41%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   56.55%   56.96%   +0.41%     
==========================================
  Files          26       26              
  Lines        4366     4403      +37     
==========================================
+ Hits         2469     2508      +39     
- Misses       1651     1653       +2     
+ Partials      246      242       -4

Impacted Files	Coverage Δ
internal/sh/shell.go	`67.76% <79.16%> (+1.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b984a6b...7db531b. Read the comment docs.

i4ki · 2017-07-10T18:33:28Z

+                exit 1; \
+        fi
+
+install: build guard-NASHPATH


I'm not sure if this is a good idea. The nash binary already looks in a default NASHPATH location if it's set and because of that new users can just install nash with 'go get' and executing it just works. I know this would be something the user will need to know in the future if he wants to create their own packages, but maybe he will only use nash in the cli for awhile or just scripts and so on.

My point is: there's several new things for new users to learn, maybe we can alleviate the burden when possible.

You are proposing to have a default NASHPATH ? Because if not I don't see a problem on make install requiring it, if the user installs with go get it will still work.

My point is that this check on make install has nothing to do with go get, I'm trying to understand the point =)

You are proposing to have a default NASHPATH?

yes.

What do you think?

Sounds good =)

i4ki

Stdlib is going awesome, I've only added some notes to discuss.

i4ki · 2017-07-10T19:03:53Z

+	mkdir -p $(NASHPATH)/lib
+	rm -f $(NASHPATH)/bin/nash
+	rm -f $(NASHPATH)/bin/nashfmt
+	cp -p ./cmd/nash/nash $(NASHPATH)/bin


I don't like this either. I expect that my ".nash" directory can be deleted with no harm, but in this case I'll lost my shell...
For example, when things goes wrong I commonly do something like that:

λ> cd ~ λ> mv .nash dotnash-bkp λ> mkdir .nash λ> nash

That works for every other shell, but will not work with nash

I see, but I can't see a way to solve that and avoid having a NASHROOT

Yeah, we'll need a NASHROOT but this variable could be inferred most of the time (i think).

For example, that's the install process I've in mind:

By go get:

$ cd ~ $ go get github.com/NeowayLabs/nash/cmd/... $ nash λ> import "fmt" λ> fmt_println("test") test λ> echo $NASHROOT /home/user/gopath/src/github.com/NeowayLabs/nash/stdlib

In the scenario above, when nash is invoked it can discover that it's installed inside $GOPATH/bin and then it checks if $GOPATH/src/github.com/NeowayLabs/nash/stdlib exists and if so, then NASHROOT must be $GOPATH/src/github.com/NeowayLabs/nash/stdlib. Otherwise it must/should warn user that NASHROOT is not set and then stdlib is not available.

By git clone && make

Golang already enforces that build must occurs inside GOPATH, then the "make install" only needs to run "go install ..." for each binary, and then everything will work as if the user had issued "go get".

By downloading a release tarball

$ cd <someplace> $ wget https://github.com/NeowayLabs/nash/releases/download/v0.5/nash-v0.5-linux-amd64.tar.gz $ tar xvf nash-v0.5-linux-amd64.tar.gz -C ./nash $ ls nash bin stdlib $ ls nash/bin nash nashfmt nashdoc $ export PATH="$PATH:<someplace>/nash/bin" $ nash λ> import "fmt" λ> fmt_println("test") test λ> echo $NASHROOT <someplace>/nash/stdlib

In the case above, nash will check if it is being ran from an exploded tarball, ie., it's inside a bin directory (outside GOPATH) and directory stdlib exists in the parent folder.

Setting NASHROOT explicitly, or install stdlib in another place

$ wget https://myserver.com/files/nash -O /usr/bin/nash $ chmod +x /usr/bin/nash $ nash λ> import "fmt" ERROR: <interactive>:1:0: Failed to import path 'fmt'. The locations below have been tried: "./fmt", "./fmt.sh", "/home/i4k/.nash/lib/fmt", "/home/i4k/.nash/lib/fmt.sh" λ> λ> curl -s https://myserver.com/files/stdlib.tar.gz | tar xvf -C $HOME+"/stdlib" - λ> setenv NASHROOT = $HOME+"/stdlib" λ> nash λ> import "fmt" λ> fmt_println("test") test λ>

If someone just don't want the stdlib, or want to put it somewhere else, or he just wants to drop the shell inside the /usr/bin of a server, then it could just use the shell without stdlib or he can set explicitly the NASHROOT to some place where it exists.

Im not very fond to this idea:

In the scenario above, when nash is invoked it can discover that it's installed inside $GOPATH/bin and then it checks if $GOPATH/src/github.com/NeowayLabs/nash/stdlib

These days Go can even work with default GOPATH (there will be no env var exported), it seems like we are mixing nash with Go too much =/. I was more prone to make it harder to install with go get but keep it more simple, no logic involving GOPATH.

If you go get nash, it will work with default NASHPATH and NASHROOT, but there will be no stdlib available. If you want the stdlib you must:

Install with make

Install release with tarball

Do it manually copying stdlib to NASHROOT

I never liked Go thing of only building inside GOPATH... I like being able to build and run nash tests where I want =), specially because I don't see a huge benefit on enforcing this.

I already disliked how my print implementation leaked Go's details, this seems to enforce it even more (what does my nash have to do with GOPATH ?). Ideally the user could never even know that nash is written in Go.

I like having default values for NASHPATH and NASHROOT, but I dislike inferring them using GOPATH.

These days Go can even work with default GOPATH (there will be no env var exported)

Yes, and then we can check if it's inside GOPATH or inside $HOME/go 😛

it seems like we are mixing nash with Go too much =/. I was more prone to make it harder to install with go get but keep it more simple, no logic involving GOPATH.

When you install with "go get" you aren't just installing but compiling from source code. Nash is written in Go, then is very appropriate to easy the installation process for people using nash from source code.

'Go get' is the first thing that came to mind when someone sees a Go project, then it's reasonable to make it work in the first place. I enjoy go-gettable projects.. would be sad that nash didn't work that way.

I already disliked how my print implementation leaked Go's details, this seems to enforce it even more (what does my nash have to do with GOPATH ?). Ideally the user could never even know that nash is written in Go.

About the print implementation leaking Go's internal format, it was a implementation design choice. In the future, that do not need to be that way.
Users that download from binary will never know that nash is written in Go... Unless they look inside the binary to find GOPATH string there or trace the system calls to know that we probe golang directories.. But if someone hexdump the binary he will see the runtime symbols anyway (if not striped).

I didn't say that we need to add docs to users rely on GOPATH... or put nash inside GOPATH and things like that. I'm only saying that we can make the life easier for people installing from source code. I love when things just works.

But I can live with that. Your call, we can experiment that way and discuss in the future if this turns out a problem.

You said this:

"
In the scenario above, when nash is invoked it can discover that it's installed inside $GOPATH/bin and then it checks if $GOPATH/src/github.com/NeowayLabs/nash/stdlib exists and if so, then NASHROOT must be $GOPATH/src/github.com/NeowayLabs/nash/stdlib. Otherwise it must/should warn user that NASHROOT is not set and then stdlib is not available.
"

If this is the behavior we will have to document that, so how we will not "add docs to users rely on GOPATH" ?

I think the main problem is that I don't see nash as much as a Go project, it is big enough for me to see as something decoupled from Go, even on how I get the binaries. If I want to build from master, we have make install, which is a more universal way to build stuff. I enjoy go get more for small tools. But to be honest this is a feeling that may not even make sense...I don't know, Kubernetes is also pretty decoupled from the fact that it is developed in Go when you deploy it, but I'm not sure if it is based only on the size of the project.

I just feel clumsy to document these things to the user, and if the behavior will be that we must to it (I hate implicit undocumented behavior).

We could document something like:

--- snip start ---
The environment variable NASHROOT points to the location of the stdlib in the system. If it is not set explicitly by user, then nash will try to find the stdlib in common places. If you're curious about where's the location of the stdlib in use by current shell session, simply type:

λ> echo $NASHROOT /home/i4k/go/src/github.com/NeowayLabs/nash/stdlib

--- snip end ---

but I'm ok with your idea. We can keep this way.

What about this:

1 - If NASHROOT is set, use it
2 - If not, use $HOME/nash
3 - If $HOME/nash do not have stdlib, use $GOPATH/github.com/NeowayLabs/nash
4 - Explode in million pieces if everything fails

I like using the make install because if I delete my entire GOPATH my nash still works perfectly, it is totally decoupled from my Go installation. But this way it still works to people who like to couple it with Go =)

i4ki · 2017-07-10T19:10:02Z


+echo "running stdlib tests"
+export NASHPATH=`pwd`
+./cmd/nash/nash ./stdlib/fmt_test.sh


i4ki · 2017-07-10T19:14:01Z

+
+fn assert_success(expected, got, status) {
+        if $status != "0" {
+                exit("1")


maybe adding a print with context about the error here, otherwise test will exit with 1 with no clue.

To make a generic assert_success with details on the status error I will have to inject the error message on the function too because the assert function do not have enough context to give any useful detail on this case. But it is a good idea, thanks =)

i4ki · 2017-07-10T19:23:13Z

+	cp -p ./cmd/nash/nash $(NASHPATH)/bin
+	cp -p ./cmd/nashfmt/nashfmt $(NASHPATH)/bin
+	rm -rf $(NASHPATH)/stdlib
+	cp -pr ./stdlib $(NASHPATH)/stdlib


I know that we'd talked about this in the past but now I think we really need a second place (another env var) to put nash standard library.

Because otherwise I can mess up with my nash installation every time I messed up my .nash directory...
For example, getting a clean new nash install will be a very intricate script because I need to reset my $NASHPATH/init and other files without touching stdlib... Today this is very easy:

λ> rm -rf .nash λ> git clone git@<someplace>/mydotnash.git .nash λ> nash

I like the idea of a NASHROOT =)

i4ki · 2017-07-10T19:25:53Z

People, I dislike the idea of ".nash" being our default directory for NASHPATH.
I would like to use only "nash" for this, maybe change this now is a good time.
What do you guys think?

vitorarins · 2017-07-11T02:30:19Z

@tiago4orion Maybe NASHPATH can be $HOME+"/nash" and NASHROOT can be $HOME+"/.nash".

i4ki · 2017-07-11T13:12:11Z

@vitorarins I dislike the idea of "hidden directories" at all. I made a proposal here:
https://github.com/NeowayLabs/nash/pull/228#discussion_r126682277
What do you think?

Guarantee isolation between tests manipulating environment variables.

i4ki · 2017-07-27T19:23:12Z

@katcipis I'm not sure either, but:

λ> import something
ERROR: <interactive>:1:0: Failed to import path 'something'. The locations below have been tried:
 "./something", "./something.sh", "/home/i4k/.nash/lib/something", "/home/i4k/.nash/lib/something.sh"

Maybe only trying to import is good, but improve the error message saying that NASHROOT is not set?

katcipis · 2017-07-27T19:25:10Z

Trying to import and improved error message seems the way to me, like @tiago4orion suggested. Anyone opposes ?

i4ki · 2017-07-31T00:45:22Z

+
+install: build guard-NASHPATH
+	@echo
+	@echo "Installing nash at: "$(NASHPATH)


this isn't the behaviour we had discussed. Or am I wrong?

missing a fix for this

i4ki · 2017-07-31T00:52:28Z

 ## Installation

-If you have Go, go-get it:
+Before proceeding to any of the install procedures it is important that you


Where will the user set this environment variable? I think this is confusing because nash will init their environment from $NASHPATH/init, but NASHPATH needs to be set in the first place... But how if nash is the login shell? You mean to place the NASHPATH env config inside /etc/profile ?

I thought more about this and I think is useful for this behaviour to work when someone wants to put their NASHPATH somewhere else other than the default path. But I think it should not be the default way of installing nash, because it requires another login shell or root access to /etc/profile.d

If you wish to improve this docs it will be welcomed, I think you understand this better =)

i4ki · 2017-07-31T00:55:09Z

+
+### Release
+
+Installing is so stupid that we provide small scripts to install it.


I do not understand this section. The user will download the source code to then invoke a script to download and install a release (binary) one?

i4ki · 2017-07-31T00:58:06Z

@@ -0,0 +1,13 @@
+#!/bin/bash


what's the use case for this script?

Installing a nash release (instead of the master)

but someone will download the entire source code to just invoke a script that downloads the binary release?
Or you mean to installations like below?

$ curl https://raw.githubusercontent.com/NeowayLabs/nash/master/hack/install/linux.sh | bash -

If this is the case, then better to update the README explaining the right way of invoking this script.

i4ki · 2017-07-31T01:44:35Z

 	return returns, newErrStopWalking()
 }

+func (shell *Shell) getNashRootFromGOPATH(preverr error) (string, error) {


NASHROOT is the name of the env var, but internally we can call this path only root. Then the function names could be something like sh.rootFromGOPATH(), sh.isValidRoot(path) and create a public one called sh.NashRoot(). What do you think?
I'm not sure if this is good, feel free to ignore.

Why a public one ?

consistency: http://godoc.org/github.com/NeowayLabs/nash#Shell.DotDir

and this could be useful when embedding nash in Go.

but feel free to ignore, we can add in the future if needed.

i4ki · 2017-07-31T01:56:35Z

+}
+
+func (shell *Shell) getNashRoot() (string, error) {
+	// TODO(katcipis): It is very annoying to load env vars, perhaps a shell.GetStringEnv ?


makes sense, maybe we can export a Getenv that returns a empty string if not found, but use an unexported one internally, like getenv that returns a bool indicating if the env does exists ?

We need the bool one internally because nash have to make sure the env exists and isn't only set to "".

You can just discards the bool in the call of Getenv

The problem is not existence checking, it is the type validation to see if the env var is a string or not, that is annoying =P. In the end I dont check and let it panic if the variable is not a string.

ah, got it. That's because nash environment vars could be other types other than strings.... we need to review this, because I've not see any advantage.

i4ki · 2017-07-31T01:58:59Z

-		return errors.NewError("NASHPATH environment variable not set on shell %s", shell.name)
-	} else if nashPath.Type() != sh.StringType {
-		return errors.NewError("NASHPATH must be n string")
+	dotDir, nashpatherr := shell.getNashPath()


libs installed in NASHPATH have precedence over NASHROOT?

just asking, I'm not sure what's the right behaviour

NASHROOT is used only to search for stdlib, there will be no lookup at NASHROOT/lib

I mean:

import "os/file"

must look first in $NASHPATH/lib or $NASHROOT/stdlib ?
The current code seems to look first in NASHPATH/lib.

Good question, no thoughts on that yet

i4ki · 2017-07-31T02:04:26Z

+	defer teardown()
+
+	nashpath := home + "/testnashpath"
+	os.Setenv("NASHPATH", nashpath)


As I'd commented in the docs file, I found good this case works this way but for real use it'll require another login shell being used to set this env var or maybe put this environment in the /etc/profile.d of the machine.

I'm not complaining about the test, only want to know from you what's your expected use case for that.

I dont use nash as the default shell, I always login on zsh and later I call nash =)

got it, my use case is tested in function TestLoadsLibFromHOMEIfNASHPATHIsUnset then kkk 😂
But is good to know this works anyway if I want to change default location in the future.

i4ki · 2017-07-31T03:17:40Z

+	defer teardown()
+
+	nashpath := home + "/testnashpath"
+	os.Setenv("NASHPATH", nashpath)


But now talking about the test =P
I don't think is good to set env vars in test. First because even though it's safe to call os.Setenv concurrently, it is not between two tests depending on changes in the environment (it will require sync between tests). Another problem is that even if we add proper locks in tests, this will require to restore the environment in a teardown in each test.

The Go environ is not the process environ, but just a map[string]string maintained in the runtime and protected by an envLock. I found this very surprisingly nice, but the environ belong to the shell and nash also has a map of their environments and we do not change the runtime environ every time our environ is changed... Well, to avoid problems, better to use the environ for their purpose, passing it around to child processes.

Maybe I've used this pattern in other tests, I'm not sure, but only remember that this is not good now,

A better approach should be to make newTestShell executing the test snippet in a new process and pass the environment variables explicitly to the child process (by os/exec.Cmd.Environ).

I'm not saying we have to make every test run in a child process, but only the ones depending on changes in the environment.

I followed the idea that was here:

https://github.com/NeowayLabs/nash/blob/master/internal/sh/shell_test.go#L44

But I wanted to isolate each test, so I set the env vars and use defer to restore them. Executing it on different processes seems better indeed.

Setenv on init is safe, but I'm pretty sure I've made something similar on other tests.
Maybe just leave this change to the future, I can address this in my next work on nash.

i4ki · 2017-07-31T03:25:21Z

+
+	return home, func() {
+		errs := []error{}
+		errs = append(errs, os.Setenv("HOME", curhome))


this mess of teardown could be avoided by passing the environment variables explicitly to the child process.

dont find it that messy, but dont like manipulating global state

this is messy =)

https://github.com/NeowayLabs/nash/blob/master/internal/sh/shell.go#L782

really? You're just manipulating global state here... 😂
I wasn't referring to the teardown function, but to the global state being manipulated in a unsafe way inside the teardown.

Regarding the link, sure it can be improved. It was a premature optimization, sorry.

i4ki · 2017-07-31T03:34:39Z


-	testDir = gopath + "/src/github.com/NeowayLabs/nash/" + "testfiles"
-	nashdPath = gopath + "/src/github.com/NeowayLabs/nash/cmd/nash/nash"
+	testDir := gopath + "/src/github.com/NeowayLabs/nash/" + "testfiles"


we are repeating "github.com/NeowayLabs/nash" in several places. I don't think this must be made now, but maybe we can add a constant string pointing to this and reuse this "name". In the future we can change to a canonical import path like "neoway.io/nash" and the change could be less troubled.

i4ki · 2017-07-31T03:36:37Z


-func init() {
-	gopath = os.Getenv("GOPATH")
+func setup(t *testing.T) (fixture, func()) {


much better 👍

i4ki

🤘

I just requested more comments regarding the use cases. Sorry if are things we already had covered but I dont remember.

katcipis · 2017-08-06T02:19:48Z

@tiago4orion is it good enough to merge ? this way we can evolve it on smaller steps (the PR is pretty big already)

i4ki · 2017-08-07T13:45:38Z

@katcipis yes, but can I change the "make install" to require NASHROOT to be set? and install stdlib there?

I'm thinking in something like:

install: build guard-NASHROOT
    @echo
    @echo "Installing nash at: $GOPATH/bin"
    @echo "Installing stdlib at: $NASHROOT"
    go install ./cmd/...
    cp -pr ./stdlib $(NASHROOT)/stdlib

Only for now, we can improve this in the next PRs.
What do you think?

katcipis · 2017-08-07T17:37:52Z

@tiago4orion I fixed that already and I could swear I pushed it...but now I see that I didnt, let me find the changes and push them =P

katcipis · 2017-08-07T17:41:25Z

@tiago4orion done. I also added default values like you suggested, so it will work without any env var set (except HOME)

katcipis added 7 commits June 29, 2017 20:35

Kickoff the stdlib

bb9aee4

Add println skeleton

bcc89e9

Add docs for fmt_println

79ec23e

Start to unify the build and install process

b210479

Add install with check

007a812

Add master install

c227d1a

Add version to nashfmt

e9f30df

katcipis assigned katcipis and i4ki Jun 30, 2017

katcipis requested review from i4ki, lborguetti and vitorarins June 30, 2017 00:41

katcipis added 2 commits June 29, 2017 21:42

remove vendor flag

0cf470f

execute NASHPATH validation only on install

ea4d50d

katcipis added 7 commits July 2, 2017 14:33

Fix release to contain bin and stdlib dirs on root

7b9653e

Add docs and automation for release install

2d9602a

Update TOCs

d5851e2

Add fmt_println implementation

227a4b8

Add stdlib on tries list

b8c3653

stdlib is the last try

679da5d

Fix stupid bug

cdd33ff

katcipis changed the title ~~WIP: Add stdlib~~ Add stdlib Jul 2, 2017

i4ki reviewed Jul 10, 2017

View reviewed changes

i4ki requested changes Jul 10, 2017

View reviewed changes

i4ki reviewed Jul 10, 2017

View reviewed changes

Isolate setenv and unsetenv for each test

e0c3a44

Guarantee isolation between tests manipulating environment variables.

katcipis added 2 commits July 23, 2017 17:13

Search stdlib on GOPATH if NASHROOT/HOME is unset

ed3ecbe

Add handling for invalid default HOME nashroot

a676f0c

i4ki mentioned this pull request Jul 27, 2017

stdbin: have standard binaries distributed with the language #230

Open

katcipis added 2 commits July 30, 2017 11:48

Add tests validating when no env vars are set

1863392

Dont fail if no env vars is set

f24c71d

i4ki reviewed Jul 31, 2017

View reviewed changes

Update env vars docs

b283b64

i4ki requested changes Jul 31, 2017

View reviewed changes

Fix make install

025680b

Remove unused guard clause

7db531b

i4ki approved these changes Aug 7, 2017

View reviewed changes

i4ki merged commit e3945b7 into master Aug 7, 2017

i4ki deleted the addStdlib branch August 7, 2017 17:44


		### Release

		Installing is so stupid that we provide small scripts to install it.

Conversation

katcipis commented Jun 30, 2017

Uh oh!

codecov-io commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i4ki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katcipis Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katcipis Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i4ki commented Jul 10, 2017

Uh oh!

vitorarins commented Jul 11, 2017

Uh oh!

i4ki commented Jul 11, 2017

Uh oh!

i4ki commented Jul 27, 2017

Uh oh!

katcipis commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i4ki Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 30, 2017 •

edited

Loading

katcipis Jul 12, 2017 •

edited

Loading

katcipis Jul 14, 2017 •

edited

Loading

i4ki Jul 31, 2017 •

edited

Loading